Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #1512, don't save edits with no clips #1517

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fixes #1512, don't save edits with no clips #1517

wants to merge 6 commits into from

Conversation

JamesUoM
Copy link
Contributor

Before allowing the edits to be saved/processed, check that there isn't a single segment that deletes the whole video. Otherwise Opencast will create a SMIL file with not clips and the editor WOH will fail.

@JamesUoM JamesUoM added the type:bug Something isn't working label Dec 11, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Dec 11, 2024
@github-actions GitHub Actions
Copy link

This pull request is deployed at test.editor.opencast.org/1517/2024-12-11_11-36-08/ .
It might take a few minutes for it to become available.

@JamesUoM JamesUoM force-pushed the f/catch-video-deletion branch from ab919e8 to f0c2479 Compare December 11, 2024 11:41
@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Dec 11, 2024
@github-actions GitHub Actions
Copy link

This pull request is deployed at test.editor.opencast.org/1517/2024-12-11_11-41-22/ .
It might take a few minutes for it to become available.

Arnei
Arnei requested changes Dec 12, 2024
View reviewed changes
Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found some things that I believe should be addressed before merging.

I'm not happy with how we present the user with the problem. Simply replacing the usual save text with a warning is not obvious at first glance and does not really make clear that it is supposed to be warning. Maybe keeping the usual text and instead adding a warning box would be better? (Could use the error boxes from appkit for that hint hint #1392). I would also prefer to keep rendering the save button, but disable it.

src/redux/videoSlice.ts Outdated Show resolved Hide resolved
src/redux/videoSlice.ts Outdated Show resolved Hide resolved
src/redux/videoSlice.ts Outdated Show resolved Hide resolved
src/i18n/locales/en-US.json Outdated Show resolved Hide resolved
@JamesUoM
Copy link
Contributor Author

Maybe keeping the usual text and instead adding a warning box would be better? (Could use the error boxes from appkit for that hint hint #1392). I would also prefer to keep rendering the save button, but disable it.

Agree with using the error boxes. But disabling buttons is not meant to be good accessible UX design - also I did try but as they are not form elements, disabling doesn't work. The aria-disabled will tell screen readers that it's disabled but doesn't stop it functioning.

@github-actions GitHub Actions
Copy link

This pull request is deployed at test.editor.opencast.org/1517/2024-12-18_14-09-53/ .
It might take a few minutes for it to become available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arnei Arnei Arnei requested changes

Assignees
No one assigned
Labels
type:bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JamesUoM @Arnei